drivers/serial/serial.c: adapt to the iovec-based api #14898

yamt · 2024-11-22T02:43:39Z

Summary

drivers/serial/serial.c: adapt to the iovec-based api

This would fix readv/writev issues mentioned in
#12674.
(only for this specific driver though. with this approach,
we basically have to fix every single drivers and
filesystems.)

Impact

Testing

Lightly tested on the serial console, using micropython REPL
on toywasm with esp32s3-devkit:toywasm, which used to be
suffered by the readv issue.

nuttxpr · 2024-11-22T02:50:04Z

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial details.

Here's a breakdown of missing information:

Summary: While the "what" is explained, the "how" is missing. The summary should explain precisely how serial.c is adapted to the iovec-based API. What code changes were made? What functions were modified?
Impact: This section is entirely empty. All impact points need to be addressed, even if the answer is "NO". For example, if no user impact is expected, explicitly state "Impact on user: NO". The developer needs to consider and document every potential impact. Given that this PR mentions fixing a bug, there's likely impact on compatibility (fixing a bug improves compatibility, arguably).
Testing: The testing description is insufficient. "Lightly tested" is vague. What specific tests were run? What commands were issued? The provided information about the platform is good, but the actual logs are missing. Empty code blocks are provided for "Testing logs before change" and "Testing logs after change" - these must be populated with actual output.

Specifically, the author needs to:

Elaborate on the implementation details in the summary.
Complete the Impact section, addressing each point individually.
Provide concrete testing details and include the actual logs before and after the change.

include/nuttx/fs/uio.h

fs/vfs/fs_read.c

fs/vfs/fs_uio.c

drivers/misc/dev_zero.c

drivers/loop/loop.c

drivers/misc/dev_null.c

fs/vfs/fs_uio.c

drivers/serial/serial.c

yamt · 2025-01-07T11:56:10Z

a reminder to myself: i have to look at the ci failure.

yamt · 2025-01-08T09:26:19Z

a reminder to myself: i have to look at the ci failure.

it was because of a wrong assertion. i removed the assertion.

yamt · 2025-01-09T07:49:11Z

rv-virt/citest64 failure looks like a stack overflow.

yamt · 2025-01-09T08:01:10Z

rv-virt/citest64 failure looks like a stack overflow.

i was able to reproduce the crash with today's master on local qemu.
#15478

yamt · 2025-01-10T06:36:51Z

rv-virt/citest64 failure looks like a stack overflow.

i was able to reproduce the crash with today's master on local qemu. #15478

rebased after #15478

yamt · 2025-01-10T10:25:38Z

rv-virt/citest64 failure looks like a stack overflow.

i was able to reproduce the crash with today's master on local qemu. #15478

rebased after #15478

it seems #15489 is also necessary.

yamt · 2025-01-14T05:13:27Z

rv-virt/citest64 failure looks like a stack overflow.

i was able to reproduce the crash with today's master on local qemu. #15478

rebased after #15478

it seems #15489 is also necessary.

rebased after #15489

fs/vfs/fs_uio.c

* Make readv/writev implementations update struct uio This can simplify partial result handling. * change the error number on the overflow from EOVERFLOW to EINVAL to match NetBSD * add a commented out uio_offset field. I used "#if 0" here as C comments can't nest. * add a few helper functions Note on uio_copyfrom/uio_copyto: although i'm not quite happy with the "offset" functionality, it's necessary to simplify the adaptation of some drivers like drivers/serial/serial.c, which (ab)uses the user-supplied buffer as a line-buffer.

This would fix readv/writev issues mentioned in apache#12674. (only for this specific driver though. with this approach, we basically have to fix every single drivers and filesystems.) Lightly tested on the serial console, using micropython REPL on toywasm with esp32s3-devkit:toywasm, which used to be suffered by the readv issue.

anchao · 2025-01-19T12:01:10Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

anchao · 2025-01-19T12:03:39Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

I don't understand the problem you mentioned in your other PR, or can you give me some code examples?

anchao · 2025-01-19T12:13:44Z

@yamt #15604

yamt · 2025-01-19T14:09:17Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

IMO, it's simpler than having two cases.

I don't understand the problem you mentioned in your other PR, or can you give me some code examples?

#12674 (comment)
it usually ends up with readv with two iovecs.

anchao · 2025-01-19T14:30:14Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

IMO, it's simpler than having two cases.

I don't understand the problem you mentioned in your other PR, or can you give me some code examples?

#12674 (comment) it usually ends up with readv with two iovecs.

@yamt setup micro-python is too heavy for me. Can you share some specific code or the calling point of readv in micro-python?

anchao · 2025-01-19T14:35:21Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

IMO, it's simpler than having two cases.

I don't understand the problem you mentioned in your other PR, or can you give me some code examples?

#12674 (comment) it usually ends up with readv with two iovecs.

@yamt setup micro-python is too heavy for me. Can you share some specific code or the calling point of readv in micro-python?

So the root cause is, if using readv to read 2 vector buffers, the second read will block and cannot exit, right?

anchao · 2025-01-19T15:21:34Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

IMO, it's simpler than having two cases.

I don't understand the problem you mentioned in your other PR, or can you give me some code examples?

#12674 (comment) it usually ends up with readv with two iovecs.

@yamt please help to review PR #15604 whether is works for you

yamt · 2025-01-20T02:43:45Z

@yamt Can you share what problem you want to resolve with this patch? I am plan to revert this PR. serial device is not suitable for using uio to implement 1 byte copy, It’s too complicate.

IMO, it's simpler than having two cases.

I don't understand the problem you mentioned in your other PR, or can you give me some code examples?

#12674 (comment) it usually ends up with readv with two iovecs.

@yamt setup micro-python is too heavy for me. Can you share some specific code or the calling point of readv in micro-python?

So the root cause is, if using readv to read 2 vector buffers, the second read will block and cannot exit, right?

yes.

github-actions bot added Area: Drivers Drivers issues Area: File System File System issues Size: M The size of the change in this PR is medium labels Nov 22, 2024

yamt force-pushed the readv-serial branch 4 times, most recently from b94fb12 to 2937e17 Compare November 22, 2024 07:06

xiaoxiang781216 reviewed Nov 23, 2024

View reviewed changes

yamt mentioned this pull request Nov 25, 2024

Reimplement readv to deal with short reads #12674

Closed

yamt force-pushed the readv-serial branch from 2937e17 to 7e31a26 Compare November 27, 2024 07:57

xiaoxiang781216 reviewed Dec 3, 2024

View reviewed changes

yamt force-pushed the readv-serial branch from 7e31a26 to 69ecff8 Compare December 25, 2024 01:26

yamt marked this pull request as draft December 25, 2024 02:58

yamt force-pushed the readv-serial branch from 69ecff8 to c8a7b7c Compare December 25, 2024 06:19

yamt marked this pull request as ready for review December 25, 2024 06:19

yamt force-pushed the readv-serial branch from c8a7b7c to 8f284e7 Compare December 25, 2024 07:53

yamt force-pushed the readv-serial branch from 8f284e7 to 5544104 Compare January 8, 2025 09:25

yamt force-pushed the readv-serial branch from 5544104 to 5f2df62 Compare January 10, 2025 06:36

Update a few comments after the recent readv/writev changes

7a6b22c

yamt force-pushed the readv-serial branch from 5f2df62 to 68d4963 Compare January 14, 2025 05:13

xiaoxiang781216 reviewed Jan 14, 2025

View reviewed changes

fs/vfs/fs_uio.c Outdated Show resolved Hide resolved

yamt force-pushed the readv-serial branch from 68d4963 to 2d40fa5 Compare January 14, 2025 08:51

xiaoxiang781216 approved these changes Jan 14, 2025

View reviewed changes

xiaoxiang781216 merged commit 0001008 into apache:master Jan 14, 2025
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/serial/serial.c: adapt to the iovec-based api #14898

drivers/serial/serial.c: adapt to the iovec-based api #14898

yamt commented Nov 22, 2024

nuttxpr commented Nov 22, 2024

yamt commented Jan 7, 2025

yamt commented Jan 8, 2025

yamt commented Jan 9, 2025

yamt commented Jan 9, 2025

yamt commented Jan 10, 2025

yamt commented Jan 10, 2025

yamt commented Jan 14, 2025

anchao commented Jan 19, 2025 •

edited

Loading

anchao commented Jan 19, 2025

anchao commented Jan 19, 2025

yamt commented Jan 19, 2025

anchao commented Jan 19, 2025

anchao commented Jan 19, 2025

anchao commented Jan 19, 2025

yamt commented Jan 20, 2025

drivers/serial/serial.c: adapt to the iovec-based api #14898

drivers/serial/serial.c: adapt to the iovec-based api #14898

Conversation

yamt commented Nov 22, 2024

Summary

Impact

Testing

nuttxpr commented Nov 22, 2024

yamt commented Jan 7, 2025

yamt commented Jan 8, 2025

yamt commented Jan 9, 2025

yamt commented Jan 9, 2025

yamt commented Jan 10, 2025

yamt commented Jan 10, 2025

yamt commented Jan 14, 2025

anchao commented Jan 19, 2025 • edited Loading

anchao commented Jan 19, 2025

anchao commented Jan 19, 2025

yamt commented Jan 19, 2025

anchao commented Jan 19, 2025

anchao commented Jan 19, 2025

anchao commented Jan 19, 2025

yamt commented Jan 20, 2025

anchao commented Jan 19, 2025 •

edited

Loading